Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pull up pushed down filters from table scan before replacing with index scan #429

Merged
merged 3 commits into from
Oct 16, 2024

Conversation

Maxxen
Copy link
Member

@Maxxen Maxxen commented Oct 14, 2024

Closes #428

This is a bit of a band-aide fix that disabled index scans if the table already has normal filters pushed down. I think its generally expected that the index scan would be way more selective, but as we don't have the chance to swap in the index scan until after normal filters are already pushed down we can't really apply it instead.

I made a quick attempt at trying to reconstruct and "pull up" the filters from the table scan again but it seems like it would be very fragile and error prone to do correctly 100% of the time, especially for more advanced queries. It's also a bit silly as we'd basically undo a lot of the work that went into pushing down the table filters to begin with and have to replace basically the whole logical get node.

The proper solution would be to improve the index interface in DuckDB core to give custom indexes a chance to properly participate in the query planning process while filters are being evaluated for pushdown. We could then also look at stats/estimated cardinalities to decide whether or not to use an index scan or not, instead of just always matching specific predicates.

We now pull up the filters from the table scan before replacing it with an index scan, ensuring index scans are applied even if other filters are present as well. This is usually the right thing to do as geometry predicates are usually much more expensive to evaluate compared to other filters that DuckDB can push down.

@Maxxen Maxxen changed the title Dont apply index scan if filters are already pushed down Pull up pushed down filters from table scan before replacing with index scan Oct 16, 2024
@Maxxen Maxxen merged commit bd27be4 into duckdb:main Oct 16, 2024
11 of 12 checks passed
@youngpm
Copy link

youngpm commented Oct 17, 2024

Awesome, thanks for the quick fix! Guessing this will show up in v1.1.3?

@Maxxen
Copy link
Member Author

Maxxen commented Oct 17, 2024

We're most likely going to hot-swap and update the extension we distribute for 1.1.2 tomorrow or early next week, so a FORCE INSTALL spatial should be all that's needed to get this fix once that's done. I just have another in-progress PR id like to sneak in first.

@youngpm
Copy link

youngpm commented Nov 4, 2024

@Maxxen did this go out yet? Just tried the force install but no luck!

@Maxxen
Copy link
Member Author

Maxxen commented Nov 5, 2024

@youngpm Sorry we didn't bump the extension for v1.1.2, but this fix should be available in the recently released v1.1.3!

@youngpm
Copy link

youngpm commented Nov 5, 2024

Ah thank you, got it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

INTERNAL Error: Attempted to access index 2 within vector of size 2 in WHERE when using spatial index
2 participants